Fix: extract method for SWD parity calculation #1700

ALTracer · 2023-12-17T13:15:49Z

Detailed description

This PR is aimed at two problems:

Problem 1: swdptap of firmware references both popcount and parity builtins to do the same task (of calculating SWD parity), and __popcountsi2(): 40 bytes is more expensive. Solution 1: create a shim/dispatcher in maths_utils, name it swd_odd_parity and call that from swdptap (the gpio-bitbanging one). This leads to cleaner code at that callsite, and less flash space occupied
Problem 2: *_swd.c hosted adapter driver backends also want to calculate parity, and also do that via both popcount and parity builtins. MSVC does not provide these builtins, only the __popcount for x86-64 optional instruction. Solution 2: incorporate this _MSC-specific builtin as well into the shim, and tweak all said hosted parity calculations to call the shim.

This PR is triggered from the discussion of #1688 two weeks ago.
I decided to name the function swd_odd_parity() for where it is going to be used. If future JTAG-PDI code also would want to use it (and attribute alias does not cut it), the reviewer/implementer of that can suggest a better name and I'll mass-rename the touched callsites (with a force-push).
Notice that I drop maths_utils.h from general.h and include it where needed by this PR. The point on making the shim static inline in the header is a valid discussion topic.

Your checklist for this pull request

I've read the Code of Conduct
I've read the guidelines for contributing to this repository
It builds for hardware native (make PROBE_HOST=native)
It builds as BMDA (make PROBE_HOST=hosted)
I've tested it to the best of my ability
My commit messages provide a useful short description of what the commits do

Closing issues

May help fix building BMDA under MSVC with HOSTED_BMP_ONLY=0 (caveat libusb/hidapi).

dragonmux

It's definitely looking good - nice job cleaning things up and rationalising them with this change!

We've spotted a couple of functional issues in review, noted below, and have a couple of name suggestions to pick from for what to name this new function, but we're happy once those are addressed to get this merged.

It's definitely a shame we don't have a nice way of building a sim suite or test suite for the maths functions so we can have in-repo proofs of their correctness, but that's an improvement to strive for in a future PR.

src/platforms/hosted/ftdi_swd.c

src/platforms/common/swdptap.c

src/maths_utils.c

src/include/maths_utils.h

src/maths_utils.c

ALTracer · 2023-12-29T19:18:05Z

Requesting next round of review.
Fixup squashing order: keep first 5 commits, incorporate last 4 commits into them: 7-8 into 1st, 6/9 into 4-5. On the other hand, I can replay this manually to make it clean.

dragonmux

This all looks good to us now - the one review note here is an optional change. Please replay the history to make it clean and we'll get this merged.

src/platforms/hosted/jlink_swd.c

* The only two users are cortexm_dwt_mask() and stlink_v2_set_frequency(). * This leads to less ccache invalidation when maths_utils.h changes (e.g. odd_parity)

* swdptap of firmware and "adapter"_swd of `hosted` all used parity/popcount * Extracting this also allows using MSVC with adapter backends * Use `uint8_t` to correspond to `hosted` libusb packet buffers

* BMF on Cortex-M3 relies on libgcc2 software builtin impls, not the x86-64-v2 popcnt insn, so adjust accordingly (parity is cheaper) * More type- and const-correctness (bmp_gpio_get/set)

* This still resolves to `__builtin_parity()` for GCC targeting Cortex-M

ALTracer · 2023-12-31T00:20:12Z

Merged fixups, propagated the rename, build-tested intermediate commits with a custom pre-commit extended hook, and rebased to latest main.
Think that's ready for final live testing for SWD parity regressions and any remaining review that the individual commits adhere to expectations. Please try it on a Tigard or something else FTDI.
There's one last __builtin_ctz remaining in ftdi.c:ftdi_swd_init(), and I have some ideas on how to go around it, but maybe not in this PR -- and I'd like MSVC build logs w.r.t. that.

dragonmux · 2023-12-31T00:26:09Z

Most excellent! Regarding MSVC, once the new Meson support lands it should become considerably easier to generate full BMDA builds under MSVC and get logs from them.

dragonmux

LGTM, approved pending testing with our Tigard. We'll merge (assuming it all works properly, but we believe it should) once that's been tested.

* Always use a parity function, not `popcount & 1` * Hide most `__builtin` usage (from entities like MSVC) * `ftdi_swd_seq_in_parity_mpsse()`: break up the bitmask calculation to enforce uint32_t intermediate * `jlink_swd_seq_in_parity()`: cosmetic: fix a typo in comment

dragonmux · 2023-12-31T02:28:00Z

We'll get this merged now it's been tested and the one bug found and fixed, good work! 🙂

dragonmux added this to the v2.0 release milestone Dec 17, 2023

dragonmux added the Enhancement General project improvement label Dec 17, 2023

dragonmux requested changes Dec 17, 2023

View reviewed changes

dragonmux reviewed Dec 29, 2023

View reviewed changes

src/platforms/hosted/jlink_swd.c Outdated Show resolved Hide resolved

ALTracer force-pushed the fix/parity-builtins branch 2 times, most recently from 344c1e7 to e5ce5ca Compare December 31, 2023 00:06

ALTracer added 4 commits December 31, 2023 03:06

general: Decouple maths_utils

c5c91e2

* The only two users are cortexm_dwt_mask() and stlink_v2_set_frequency(). * This leads to less ccache invalidation when maths_utils.h changes (e.g. odd_parity)

maths_utils: Factor out calculate_odd_parity() with builtins

4e42c24

* swdptap of firmware and "adapter"_swd of `hosted` all used parity/popcount * Extracting this also allows using MSVC with adapter backends * Use `uint8_t` to correspond to `hosted` libusb packet buffers

swdptap: Replace popcount with parity

fe3405f

* BMF on Cortex-M3 relies on libgcc2 software builtin impls, not the x86-64-v2 popcnt insn, so adjust accordingly (parity is cheaper) * More type- and const-correctness (bmp_gpio_get/set)

swdptap: Abstract away usage of parity builtin

5f32305

* This still resolves to `__builtin_parity()` for GCC targeting Cortex-M

ALTracer force-pushed the fix/parity-builtins branch from e5ce5ca to c812d4c Compare December 31, 2023 00:11

ALTracer force-pushed the fix/parity-builtins branch from c812d4c to 3f47538 Compare December 31, 2023 00:23

dragonmux previously approved these changes Dec 31, 2023

View reviewed changes

ALTracer dismissed dragonmux’s stale review via d7455f5 December 31, 2023 02:24

ALTracer force-pushed the fix/parity-builtins branch from 3f47538 to d7455f5 Compare December 31, 2023 02:24

dragonmux approved these changes Dec 31, 2023

View reviewed changes

dragonmux merged commit d7767d4 into blackmagic-debug:main Dec 31, 2023
6 checks passed

ALTracer deleted the fix/parity-builtins branch April 26, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: extract method for SWD parity calculation #1700

Fix: extract method for SWD parity calculation #1700

ALTracer commented Dec 17, 2023 •

edited

Loading

dragonmux left a comment

ALTracer commented Dec 29, 2023

dragonmux left a comment

ALTracer commented Dec 31, 2023

dragonmux commented Dec 31, 2023

dragonmux left a comment

dragonmux commented Dec 31, 2023

Fix: extract method for SWD parity calculation #1700

Fix: extract method for SWD parity calculation #1700

Conversation

ALTracer commented Dec 17, 2023 • edited Loading

Detailed description

Your checklist for this pull request

Closing issues

dragonmux left a comment

Choose a reason for hiding this comment

ALTracer commented Dec 29, 2023

dragonmux left a comment

Choose a reason for hiding this comment

ALTracer commented Dec 31, 2023

dragonmux commented Dec 31, 2023

dragonmux left a comment

Choose a reason for hiding this comment

dragonmux commented Dec 31, 2023

ALTracer commented Dec 17, 2023 •

edited

Loading